Automatically set EncodingType to url #726
Conversation
| # This is needed because we are passing url as the encoding type. Since the | ||
| # paginator is based on the key, we need to handle it before it can be | ||
| # round tripped. | ||
| if 'Contents' in parsed and parsed.get('EncodingType') == 'url': |
There was a problem hiding this comment.
I wonder if we should only automatically decode the URI encoded values if the EncodingType was not explicitly set by the user? For example, if a user actually looks at the S3 API and sees that you can set and EncodingType, then they might reasonably expect that the results they receive are encoded... In fact, that's probably the behavior they are expecting right now. If we automatically start decoding these, it would be a breaking change.
There was a problem hiding this comment.
There's not really any way to know that though.
There was a problem hiding this comment.
Yeah, I think we'd have to add support for that in order to make it backwards compatible.
JordonPhillips
force-pushed
the
s3_ls_encoding_type
branch
from
4e88b5a to
3d89b31
Compare
JordonPhillips
added
the
pr/needs-review
| @@ -176,12 +176,12 @@ def test_can_delete_urlencoded_object(self): | |||
| bucket_contents = self.client.list_objects( | |||
| Bucket=self.bucket_name)['Contents'] | |||
| self.assertEqual(len(bucket_contents), 1) | |||
| self.assertEqual(bucket_contents[0]['Key'], 'a+b/foo') | |||
| self.assertEqual(bucket_contents[0]['Key'], u'a+b/foo') | |||
There was a problem hiding this comment.
Why do you need to add the unicode prefix to these? I feel like these should still pass with the unicode prefixed removed.
|
Implementation looks fine. It would be nice to have an integration test for this by adding a key that would normally break the xml parser and show how it is now fixed with autopopulation of encoding type and also show that it is still url encoded (not automatically decoded) if the user specified it. |
| self.create_object(key_name) | ||
| parsed = self.client.list_objects(Bucket=self.bucket_name) | ||
| self.assertEqual(len(parsed['Contents']), 1) | ||
| self.assertEqual(parsed['Contents'][0]['Key'], key_name) |
There was a problem hiding this comment.
Could we also do a second call where we specify EncodingType=url and show that it is not being url decoded? You could just add it to this test.
|
Just one more thing. It should be good to merge. Also make sure you have a PR for the CLI to update it so we can merge this and the corresponding CLI PR to avoid test failures when we just merge this PR. |
JordonPhillips
force-pushed
the
s3_ls_encoding_type
branch
from
4f0687c to
b69f60b
Compare
|
👍 |
|
Thanks 🚢. We can merge this when the CLI PR is made to update the CLI based off of these changes. |
|
|
| # paginator is based on the key, we need to handle it before it can be | ||
| # round tripped. | ||
| if 'Contents' in parsed and parsed.get('EncodingType') == 'url' and \ | ||
| kwargs['context'].get('EncodingTypeAutoSet') == True: |
There was a problem hiding this comment.
and context.get('EncodingTypeAutoSet', False): or just:
and context.get('EncodingTypeAutoSet')
|
|
JordonPhillips
force-pushed
the
s3_ls_encoding_type
branch
from
a9d9971 to
18a590e
Compare
Automatically set EncodingType to url
Following boto/botocore#726, list_objects will automatically include a encoding-type=url query param. However, the client does not decode all of the response elements properly -- notably, Prefix would remain encoded. Hopefully this will be fixed soon-ish (I've got a patch proposed at boto/botocore#1901) but in the meantime, use the work-around suggested in boto/boto3#816 of unregistering the set_list_objects_encoding_type_url handler. Signed-off-by: Tim Burke <tim.burke@gmail.com>
Following boto/botocore#726, list_objects will automatically include a encoding-type=url query param. However, the client does not decode all of the response elements properly -- notably, Prefix would remain encoded. Hopefully this will be fixed soon-ish (I've got a patch proposed at boto/botocore#1901) but in the meantime, use the work-around suggested in boto/boto3#816 of unregistering the set_list_objects_encoding_type_url handler. Signed-off-by: Tim Burke <tim.burke@gmail.com>
Some unicode values can break the XML parser, so we have s3 url encode the keys and decode them on our end.